Skip to content

fix: Add Cython build step to Makefile #2869

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
kevinjqliu merged 2 commits into from
Dec 28, 2025
Merged

fix: Add Cython build step to Makefile #2869

kevinjqliu merged 2 commits into from
Dec 28, 2025
+1 −1

Conversation

@geruh
Copy link
Contributor

@geruh geruh commented Dec 28, 2025 •
edited
Loading

Rationale for this change

Currently, uv sync is building the Cython extensions on first install. After running make clean (which deletes .so files), subsequent uv sync does not rebuild them, causing:

  ModuleNotFoundError: No module named 'pyiceberg.avro.decoder_fast'

This seems to be related to some known uv behavior: astral-sh/uv#12399. The --reinstall flag forces uv to rebuild the package and its extensions.

Note: CI works because it always starts fresh. Local dev breaks after make clean.

Are these changes tested?

make clean && make test

Are there any user-facing changes?

new make target

@geruh geruh marked this pull request as ready for review December 28, 2025 19:20
kevinjqliu
kevinjqliu approved these changes Dec 28, 2025
View reviewed changes
Copy link
Contributor

@kevinjqliu kevinjqliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TY! Interesting uv behavior.

I tested this locally with

make clean && make install && PYTEST_ARGS='-k tests/avro/test_decoder.py' make test

this fails on current main because the cython extension is not built, but passes here

@kevinjqliu
Copy link
Contributor

kevinjqliu commented Dec 28, 2025

the cython extension fails silently in dev setup

iceberg-python/setup.py

Lines 39 to 72 in b0a7878

allowed_to_fail = os.environ.get("CIBUILDWHEEL", "0") != "1"
ext_modules = []
try:
import Cython.Compiler.Options
from Cython.Build import cythonize
Cython.Compiler.Options.annotate = True
if os.name == "nt": # Windows
extra_compile_args = ["/O2"]
else: # UNIX-based systems (Linux, macOS)
extra_compile_args = ["-O3"]
package_path = "pyiceberg"
extensions = [
Extension(
"pyiceberg.avro.decoder_fast",
[os.path.join(package_path, "avro", "decoder_fast.pyx")],
extra_compile_args=extra_compile_args,
language="c",
)
]
ext_modules = cythonize(
extensions,
include_path=[package_path],
compiler_directives={"language_level": "3"},
annotate=True,
)
except Exception:
if not allowed_to_fail:

but will throw when fail during artifact build.

iceberg-python/.github/workflows/pypi-build-artifacts.yml

Lines 63 to 78 in b0a7878

- name: Build wheels
uses: pypa/cibuildwheel@v3.3.0
with:
output-dir: wheelhouse
config-file: "pyproject.toml"
env:
# Ignore 32 bit architectures
CIBW_ARCHS: "auto64"
CIBW_PROJECT_REQUIRES_PYTHON: ">=3.10"
CIBW_TEST_REQUIRES: "pytest==7.4.2 moto==5.0.1"
CIBW_TEST_COMMAND: "pytest {project}/tests/avro/test_decoder.py"
# Ignore tests for pypy since not all dependencies are compiled for it
# and would require a local rust build chain
CIBW_TEST_SKIP: "pp*"
# Skip free-threaded (PEP 703) builds until we evaluate decoder_fast support
CIBW_SKIP: "cp3*t-*"

@kevinjqliu kevinjqliu merged commit 59cdf33 into apache:main Dec 28, 2025
8 checks passed
@geruh geruh deleted the cyth branch January 2, 2026 22:48
@kevinjqliu kevinjqliu mentioned this pull request Jan 29, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kevinjqliu kevinjqliu kevinjqliu approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@geruh @kevinjqliu